Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors builder context management by moving context pushing/popping into Block methods rather than handling it at the top-level builder. It also changes how initializer values are stored (as raw values rather than pre-converted expressions) and makes array element naming context-explicit.
Key Changes
- Context management moved from
elaborate_toplevelinto_elaborated_def_to_protoand_generated_def_to_protomethods - Initializer values stored as raw types and converted to expressions only when serializing to protobuf
_name_of_childmethod now takes explicit context parameter instead of relying onbuilder.get_enclosing_block()
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| edg/core/Builder.py | Refactored pop_to to check early-exit conditions and removed context management from elaborate_toplevel |
| edg/core/Blocks.py | Added context push/pop in _elaborated_def_to_proto with try-finally block |
| edg/core/Generator.py | Added context push/pop in _generated_def_to_proto with try-finally block |
| edg/core/DesignTop.py | Added context push/pop in _elaborated_def_to_proto and updated _name_of_child calls with explicit context |
| edg/core/HierarchyBlock.py | Changed to store raw initializer values and convert them when serializing; updated _name_of_child calls |
| edg/core/Binding.py | Updated InitParamBinding.value type annotation from Optional[ConstraintExpr] to Optional[Any] |
| edg/core/Core.py | Added context parameter to _name_of_child signature |
| edg/core/Array.py | Updated _name_of_child to use explicit context instead of builder.get_enclosing_block() |
| edg/core/Ports.py | Simplified adapter naming to use full path from enclosing block |
| edg/abstract_parts/IoController.py | Updated _name_of_child call with explicit context parameter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (elt is None and not self.stack) or (elt is not None and self.stack[-1] is elt): | ||
| return | ||
|
|
||
| self.stack.pop() |
There was a problem hiding this comment.
The pop_to method only pops a single element from the stack, but should continue popping until reaching the target element. The original implementation used a while loop to pop multiple elements. If there are nested contexts, this will fail to restore the stack to the correct state.
Consider restoring the while loop:
while (elt is None and self.stack) or (elt is not None and self.stack[-1] is not elt):
self.stack.pop()| self.stack.pop() | |
| while (elt is None and self.stack) or (elt is not None and self.stack[-1] is not elt): | |
| self.stack.pop() |
1 participant
Move context pushing for contents, generate into Block methods, instead of the top-level builder method. This becomes more consistent with Block metaclass init hook handling the
__init__context.Also changes initializers to be stored as the raw value, instead of _to_expr_type.
Adds explicit context to _name_from, since array element naming conventions are dependent on if accessing from outside (request required) or from inside (explicit elements required). Previously, this depended on the builder context.